OCPBUGS-78542: fix useHistory crash and null safety guards#178
OCPBUGS-78542: fix useHistory crash and null safety guards#178upalatucci wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@upalatucci: This pull request references Jira Issue OCPBUGS-78542, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis pull request introduces defensive null-safety handling in utility functions and migrates React Router navigation from the deprecated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: upalatucci The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/nodenetworkconfiguration/components/TopologySidebar/CreatePolicyDrawer.tsx (1)
78-82:⚠️ Potential issue | 🟠 MajorUse absolute path and construct query parameters consistently
Line 80 uses a relative path and direct template interpolation for the query parameter, which is inconsistent with other navigate calls in the codebase (lines 32 and 90 use absolute paths like
/node-network-configuration). The preferred pattern in this codebase (TopologyToolbar.tsx line 47) is to useURLSearchParamswith the location object format. This ensures consistent routing behavior and proper URL encoding.Suggested fix
navigate( createAnotherPolicy - ? `node-network-configuration?createPolicy=true&physicalNetworkName=${networkName}` + ? `${createAnotherPolicy ? `/node-network-configuration?createPolicy=true&physicalNetworkName=${encodeURIComponent(networkName)}` : ''}`Or preferably, match the TopologyToolbar pattern:
+ const navigationParams = new URLSearchParams({ + [CREATE_POLICY_QUERY_PARAM]: 'true', + 'physicalNetworkName': networkName, + }); navigate( createAnotherPolicy - ? `node-network-configuration?createPolicy=true&physicalNetworkName=${networkName}` + ? { pathname: '/node-network-configuration', search: navigationParams.toString() } : getResourceUrl({ model: NodeNetworkConfigurationPolicyModel, resource: createdPolicy }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/nodenetworkconfiguration/components/TopologySidebar/CreatePolicyDrawer.tsx` around lines 78 - 82, The navigate call that builds the URL when createAnotherPolicy is true uses a relative path and direct interpolation of networkName; change it to use the absolute path "/node-network-configuration" and construct query parameters via URLSearchParams (or the location object form) to match the project's pattern (see TopologyToolbar). Update the ternary branch in the navigate invocation that references createAnotherPolicy to build the destination like { pathname: '/node-network-configuration', search: new URLSearchParams({ createPolicy: 'true', physicalNetworkName: networkName }).toString() } while leaving the existing getResourceUrl(NodeNetworkConfigurationPolicyModel, createdPolicy) branch intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/resources/enactments/utils.ts`:
- Around line 3-4: getEnactmentStatus currently can return undefined even though
it's typed as string; update the function (getEnactmentStatus with parameter
V1beta1NodeNetworkConfigurationEnactment) to return a concrete default status
when no condition with status === 'True' is found (e.g., return 'Unknown' or
'Pending') so callers always receive a string and downstream grouping/filtering
won't break.
In `@src/views/nodenetworkconfiguration/utils/utils.ts`:
- Around line 224-225: The function getCorrelatedEnactment currently declares a
non-optional return but uses .find() and can return undefined; change its
signature to return the NNCE type as optional (e.g., NNCE | undefined) and
update any parameter types that assume a non-null value accordingly, and then
update callers such as getNNCEConfiguredInterfaces to accept the optional return
(adjust its parameter type to accept undefined) so the type contract matches the
runtime null-safe behavior.
---
Outside diff comments:
In
`@src/views/nodenetworkconfiguration/components/TopologySidebar/CreatePolicyDrawer.tsx`:
- Around line 78-82: The navigate call that builds the URL when
createAnotherPolicy is true uses a relative path and direct interpolation of
networkName; change it to use the absolute path "/node-network-configuration"
and construct query parameters via URLSearchParams (or the location object form)
to match the project's pattern (see TopologyToolbar). Update the ternary branch
in the navigate invocation that references createAnotherPolicy to build the
destination like { pathname: '/node-network-configuration', search: new
URLSearchParams({ createPolicy: 'true', physicalNetworkName: networkName
}).toString() } while leaving the existing
getResourceUrl(NodeNetworkConfigurationPolicyModel, createdPolicy) branch
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f348e9b3-aee3-421d-8146-36eb2c7583ca
📒 Files selected for processing (17)
src/utils/components/PolicyForm/PolicyWizard/utils/hooks/useNodeInterfaces/utils/utils.tssrc/utils/resources/enactments/utils.tssrc/utils/resources/policies/utils.tssrc/views/nodenetworkconfiguration/Topology.tsxsrc/views/nodenetworkconfiguration/components/TopologySidebar/CreatePolicyDrawer.tsxsrc/views/nodenetworkconfiguration/components/TopologySidebar/InterfaceDrawer/TopologyDrawer.tsxsrc/views/nodenetworkconfiguration/components/TopologyToolbar/TopologyToolbar.tsxsrc/views/nodenetworkconfiguration/utils/utils.tssrc/views/physical-networks/components/PhysicalNetworksEmptyState.tsxsrc/views/physical-networks/components/PhysicalNetworksPageHeader.tsxsrc/views/physical-networks/components/PhysicalNetworksTable/components/PhysicalNetworkRow/PhysicalNetworkRow.tsxsrc/views/physical-networks/components/PhysicalNetworksTable/components/PhysicalNetworkRow/utils/utils.tsxsrc/views/policies/actions/PolicyActions.tsxsrc/views/policies/components/DeleteModal.tsxsrc/views/policies/list/components/CreatePolicyButtons.tsxsrc/views/policies/new/NewPolicy.tsxsrc/views/states/list/hooks/useDrawerInterface.ts
| export const getEnactmentStatus = (enactment: V1beta1NodeNetworkConfigurationEnactment): string => | ||
| enactment?.status?.conditions?.find((condition) => condition.status === 'True').type; | ||
| enactment?.status?.conditions?.find((condition) => condition.status === 'True')?.type; |
There was a problem hiding this comment.
Return a concrete default status instead of allowing undefined
Line 4 can now evaluate to undefined, but getEnactmentStatus is typed as string and callers use the result as a status key/value. This can silently create undefined entries and break filtering/state grouping behavior downstream.
Suggested fix
export const getEnactmentStatus = (enactment: V1beta1NodeNetworkConfigurationEnactment): string =>
- enactment?.status?.conditions?.find((condition) => condition.status === 'True')?.type;
+ enactment?.status?.conditions?.find((condition) => condition.status === 'True')?.type ?? 'Pending';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getEnactmentStatus = (enactment: V1beta1NodeNetworkConfigurationEnactment): string => | |
| enactment?.status?.conditions?.find((condition) => condition.status === 'True').type; | |
| enactment?.status?.conditions?.find((condition) => condition.status === 'True')?.type; | |
| export const getEnactmentStatus = (enactment: V1beta1NodeNetworkConfigurationEnactment): string => | |
| enactment?.status?.conditions?.find((condition) => condition.status === 'True')?.type ?? 'Pending'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/resources/enactments/utils.ts` around lines 3 - 4,
getEnactmentStatus currently can return undefined even though it's typed as
string; update the function (getEnactmentStatus with parameter
V1beta1NodeNetworkConfigurationEnactment) to return a concrete default status
when no condition with status === 'True' is found (e.g., return 'Unknown' or
'Pending') so callers always receive a string and downstream grouping/filtering
won't break.
| return availableEnhancments?.find((nnce) => | ||
| nnce.metadata?.ownerReferences?.some((ref) => ref.name === nnsName), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Function signature and return expression =="
rg -n -C2 'const getCorrelatedEnactment|availableEnhancments\?\.\s*find|V1beta1NodeNetworkConfigurationEnactment' src/views/nodenetworkconfiguration/utils/utils.ts
echo
echo "== TypeScript strictness flags in tsconfig files =="
fd 'tsconfig.*json' -t f | while read -r f; do
echo "-- $f --"
rg -n '"strict"\s*:|"strictNullChecks"\s*:' "$f" || true
doneRepository: openshift/nmstate-console-plugin
Length of output: 1204
🏁 Script executed:
# Search for all usages of getCorrelatedEnactment
rg -n 'getCorrelatedEnactment' src/views/nodenetworkconfiguration/utils/utils.ts
# Also check if it's exported and used elsewhere
rg -r 'getCorrelatedEnactment' src/ --type ts --type tsxRepository: openshift/nmstate-console-plugin
Length of output: 239
🏁 Script executed:
# Get context around the function call at line 179 to see how the result is used
rg -n -A 10 -B 5 'const enhancment = getCorrelatedEnactment' src/views/nodenetworkconfiguration/utils/utils.tsRepository: openshift/nmstate-console-plugin
Length of output: 702
🏁 Script executed:
# Get the full implementation of getNNCEConfiguredInterfaces
rg -n -A 20 'const getNNCEConfiguredInterfaces' src/views/nodenetworkconfiguration/utils/utils.tsRepository: openshift/nmstate-console-plugin
Length of output: 793
Type contract no longer matches actual return value.
getCorrelatedEnactment can return undefined when .find() yields no match, but the function signature declares a non-optional return type. The downstream caller getNNCEConfiguredInterfaces defends against this with an isEmpty check, but this masks a type contract mismatch that weakens maintainability. Align both the parameter and return types with the null-safe behavior:
Proposed fix
const getCorrelatedEnactment = (
- availableEnhancments: V1beta1NodeNetworkConfigurationEnactment[],
+ availableEnhancments: V1beta1NodeNetworkConfigurationEnactment[] | undefined,
nnsName: string,
-): V1beta1NodeNetworkConfigurationEnactment => {
+): V1beta1NodeNetworkConfigurationEnactment | undefined => {
return availableEnhancments?.find((nnce) =>
nnce.metadata?.ownerReferences?.some((ref) => ref.name === nnsName),
);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/nodenetworkconfiguration/utils/utils.ts` around lines 224 - 225,
The function getCorrelatedEnactment currently declares a non-optional return but
uses .find() and can return undefined; change its signature to return the NNCE
type as optional (e.g., NNCE | undefined) and update any parameter types that
assume a non-null value accordingly, and then update callers such as
getNNCEConfiguredInterfaces to accept the optional return (adjust its parameter
type to accept undefined) so the type contract matches the runtime null-safe
behavior.
|
@upalatucci: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
useHistory(React Router v5) calls touseNavigatefromreact-router-dom-v5-compat, fixinguseHistory is not a functionruntime crash in the console pluginuseSignalEffectreturningnullinstead ofundefinedas cleanup functionTypeError: n is not iterablecrashes when spreading potentially undefined values (interfaces,ports,ownerReferences,nodes).find().typeaccess ingetEnactmentStatuswith optional chainingTest plan
useHistory is not a functionerrorn is not iterableerroruseNavigateMade with Cursor